-
Notifications
You must be signed in to change notification settings - Fork 6.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhancement: added navItem hover #6775
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Thanks for the PR. Personally I think this is a nice addition - I'm interested to hear others' thoughts |
+1 for theses change, I remember we already have discussion about hover effect but I didn't know in which pr. |
I thought we discussed it before and decided not to do this. What changed now? 🤔 |
I used to agree with that. And if I remember correctly last time the proposal was that the hover effect should be the same as the active style for navigation, which was for ‘heavy’. |
I do not think the links in the header and footer navigation items are buttons, therefore I do not think it is right for these elements to behave like buttons in the hover state Here is why; Maybe changing the text color would be better on hover, I'm not 100% against it as the current solution only works on hover 🙈 |
Agreed, and that's what I said on the last PR. This should at maximum change the text on hover, but this is a link, not a button. |
and why not just underline it when hover it's discreet and allow user to know there are possible interaction. |
The footer links have this same effect on hover, should i go with underline or text color change and should i do it for the top navBar or both it and the footer? |
Since we use horizontal lines to divide the design into sections(header, content, footer), I think the use of underlines in the texts would be overkill / retro look 👀 Wouldn't it be enough to indicate it with text color and cursor pointer? |
@canerakdas I made the text color change to the nodejs green (text-green-400) on hover in both dark and light mode, its visible in both modes |
I'm still very much a beginner but i have a take on this that i'm not sure if valid or not so please correct me on this: |
Unfortunately, not. For accessibility standards, the contrast on the background color is way more important. There are a bunch of reasons why, and I believe you could study those, as these are standard UX and accessibility practices. Maybe this blog post can help you out :) https://medium.com/@think_ui/why-color-contrast-is-not-as-black-and-white-as-it-seems-94197a72b005 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@ovflowd I know you are busy and feel free to ignore, but curious if you are accepting of this change. We've gone around and around, but want to make sure you had a chance to review it too. if I don't hear from you, I will merge tomorrow |
Appreciate tagging me. Honestly speaking I'm a -1 for the current proposal (how the PR stands) But I won't go out my way to block the PR if I get reassurance the concerns Caner and I shared initially get addressed in a follow-up. I understand the why's, but these are not buttons, but links. Buttons do actions. Links do navigation. Anyhow, feel free to merge. |
I tend to agree with Claudio, but with these changes the footer and header will be in line. |
It's less about being clear and more about being accessible and compliant imo |
it is keyboard accessible. the heart of the linked resource suggests that actual controls, like |
Lighthouse Results
|
@mAmineChniti can you sync your fork with |
Done. |
thanks @mAmineChniti for coming with us on this journey. I hope you find more ways to contribute to the project! |
Description
Just a very simple change, added a couple of lines of css to have the top navBar that has Learn, About, etc.. have the same hover effect as the footer bar
Validation
Related Issues
This was suggested in:
#6752
Check List
npm run format
to ensure the code follows the style guide.npm run test
to check if all tests are passing.npx turbo build
to check if the website builds without errors.